-
-
Notifications
You must be signed in to change notification settings - Fork 17
Glasgow | Sheetal Kharab | Module-Decomposition | Sprint 4 | prep-exercises #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Glasgow | Sheetal Kharab | Module-Decomposition | Sprint 4 | prep-exercises #28
Conversation
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
6 similar comments
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
bankAccount.py
Outdated
| @@ -0,0 +1,32 @@ | |||
| def open_account(balances, name, amount): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of the exercise is not to just find a bug, but use mypy to do that. Could you please add types to this file and others as well? You will need to add mypy to requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. I have already added type annotations to all functions in this file and other related files and used mypy to check for type consistency. The mypy check shows Success: no issues found.
I realized I had forgotten to include mypy in requirements.txt, but I’ve now added it so it will be installed automatically.
dataclassPerson.py
Outdated
| today_date = date.today() | ||
| birth_date= self.date_of_birth | ||
| #to check date and month as well | ||
| age = today_date.year-birth_date.year- ((today_date.month, today_date.day) < (birth_date.month, birth_date.day)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the readability could be improved if you use date arithmetics here, for example (assuming today = date.today())
date(
today.year - 18,
today.month,
today.day
)
and compare it with the date of birth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried to follow your suggestion and made changes.
enums.py
Outdated
| age_input = input("Enter your age: ") | ||
| if not age_input.isdigit(): | ||
| print("Error: Age must be a number.", file=sys.stderr) | ||
| continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider throwing a value error here and using try/catch block instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, try to follow your suggestion and made changes
| Person(name="Eliza", age=34, preferred_operating_system=OperatingSystem.ARCH), | ||
| ] | ||
|
|
||
| laptops = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: you can consider using ruff to improve your code formatiing, something like:
pip install ruff
ruff format .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do it definitely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you get a chance to check this out?
| person = Person(name=name, age=age, preferred_operating_system=preferred_os) | ||
| people.append(person) | ||
|
|
||
| # Find matching laptops and print count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding comments, try to see where it make sense to group these various bits of logic into methods with descriptive names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to make changes and use different function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! In general, apart from invoking the main method, all the logic needs to be part of some method / class. The clarity should be coming from the name of the methods and structure of the code rather than comments explaining what you are doing (those could be useful too in some cases but should not be used as a substitute for clear structure and naming).
That said, from what I see this is more like a hands-on exercise to practice some concepts so it is ok your code has less structure.
enums.py
Outdated
| best_os = OperatingSystem.MACOS | ||
| best_count = mac_count | ||
|
|
||
| if arch_count > best_count: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should design it in a way that doesn't require you to go through all possible values manually. Imagine your OperatingSystem enum containing 50 different systems. Try to use, for example, a dict structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried to follow your suggestion
dataclassPerson.py
Outdated
| @@ -0,0 +1,21 @@ | |||
| from dataclasses import dataclass | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In python, the naming convention for files is snake_case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I changed them thank you
DaryaShirokova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! A few comments, most are just informational :)
| Person(name="Eliza", age=34, preferred_operating_system=OperatingSystem.ARCH), | ||
| ] | ||
|
|
||
| laptops = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you get a chance to check this out?
dataclass_person.py
Outdated
| date_of_birth: date | ||
| preferred_operating_system: str | ||
|
|
||
| def is_adult(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to add this as a standalone method or as a part of the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback. Yes, is_adult should be a method inside the Person class. I accidentally placed it outside. I’ll move it inside the class so it can be called as person.is_adult().
| total += pence | ||
| return total | ||
|
|
||
| def format_pence_as_string(total_pence: int)-> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting is a bit off in places which makes the code slightly harder to read. Using ruff or other auto formatters might help with readability (e.g. int)-> missing space)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you I installed ruff and did it.
| person = Person(name=name, age=age, preferred_operating_system=preferred_os) | ||
| people.append(person) | ||
|
|
||
| # Find matching laptops and print count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! In general, apart from invoking the main method, all the logic needs to be part of some method / class. The clarity should be coming from the name of the methods and structure of the code rather than comments explaining what you are doing (those could be useful too in some cases but should not be used as a substitute for clear structure and naming).
That said, from what I see this is more like a hands-on exercise to practice some concepts so it is ok your code has less structure.
| def parse_operating_system(s: str) -> Optional[OperatingSystem]: | ||
| s_normal = s.strip().lower() | ||
| for os in OperatingSystem: | ||
| if os.value.lower() == s_normal: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment (no need to fix) - you to made it case insensitive so it works (all good), but overall you could also use https://docs.python.org/3/library/enum.html#enum.EnumType.__getitem__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the note!I’ll keep that in mind for future refactoring.
|
|
||
|
|
||
| def find_possible_laptops(laptops: List[Laptop], person: Person) -> List[Laptop]: | ||
| possible_laptops = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for you information, an alternative approach could also be
return [
laptop for laptop in laptops
if laptop.operating_system == person.preferred_operating_system
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! yes a list comprehension is more concise and readable
|
@DaryaShirokova , Please can i change label reviewed to complete? |
|
Let me
Let me have a quick look tomorrow or on Saturday, I was a bit busy the last couple of days :) |
Self checklist
Changelist
I pushed all my prep exercise change
Questions
no